Skip to content

Add "Go to type" hyperlinks in the hover popup (like Rust has) #4691

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add "Go to type" hyperlinks in the hover popup (like Rust has) #4691

wants to merge 3 commits into from
+146 −45

Conversation

dnikolovv
Copy link

@dnikolovv dnikolovv commented Aug 13, 2025 •
edited
Loading

go-to-types

This is how it looks in Rust:

image

@dnikolovv dnikolovv requested a review from wz1000 as a code owner August 13, 2025 16:39
@dnikolovv dnikolovv changed the title Add "Go to type" hyperlinks in the hover popup. Add "Go to type" hyperlinks in the hover popup (like Rust has) Aug 13, 2025
@dschrempf
Copy link
Collaborator

Thanks! I had a quick look at the popups in my editor, and the "Go to ..." renders well and looks great. A few questions/suggestions:

  • Can we sort the list of types in the same order they appear in the type signature of the highlighted definition?
  • Does it make sense to write "Go to type ..." because we are only listing types afterwards.
  • The hint only shows up when symbols on the value-level are highlighted. (See attached screenshots).
image image

@dnikolovv
Copy link
Author

Hi @dschrempf

  • Can we sort the list of types in the same order they appear in the type signature of the highlighted definition?

I need to check if the order they come in is deterministic when we query the locations.

  • Does it make sense to write "Go to type ..." because we are only listing types afterwards.

Yes, I just copied the Rust one.

  • The hint only shows up when symbols on the value-level are highlighted. (See attached screenshots).

Yeah. For some reason the locations query does not return anything when you're on the type level. This is how Go To Definition works right now. It might be a bug though?

@dschrempf
Copy link
Collaborator

Ad "Go to type ... not showing for symbols on the type level": Actually, it makes sense, but only somewhat.

When the cursor is on X (see picture)
image

  • Go to definition goes to the definition of the data type X. That's good.
  • Go to type does nothing (error: Not found for: X) -> Makes sense, the type of X is Type, or Star, we could go there.

However, when the cursor is on x on the type level (see picture)
image

  • Go to definition goes to x on the value level (one line below). Does this make sense? I guess it does.
  • Go to type gives the same error as above (error: Not found for: x). This should actually go to X, or suggest a list of types involved in defining x, shouldn't it? And this is also why you do not get a list of types @dnikolovv

@dnikolovv
Copy link
Author

@dschrempf

Go to type gives the same error as above (error: Not found for: x). This should actually go to X, or suggest a list of types involved in defining x, shouldn't it? And this is also why you do not get a list of types @dnikolovv

Exactly - the definitions in the hover use the same functionality underneath. I don't know if this is a bug or intended behavior. Maybe @fendor knows?

@fendor
Copy link
Collaborator

fendor commented Aug 14, 2025

I don't know exactly, I presume it is an artefact of HIE file structure or how GHC defines the AST and its source locations. So, probably a "bug".

I don't think it is something worth investing too much time into right now (i.e., not for this PR in particular), and we should rather focus on the presentation in the hover.

@dnikolovv
Copy link
Author

Not sure why this pipeline fails. Running cabal test ghcide-tests locally with GHC 9.12.2 gives me All 398 tests passed (287.79s).

@fendor
Copy link
Collaborator

fendor commented Aug 16, 2025

@dnikolovv The testsuite in HLS is unfortunately quite flaky. You can likely ignore the testcase, and wait for an admin to restart the job. If the failing test case cannot be reproduced locally, then you can ignore it for now.

jian-lin
jian-lin reviewed Aug 17, 2025
View reviewed changes
ghcide/src/Development/IDE/Spans/AtPoint.hs
Comment on lines -268 to +288
pTypes :: [T.Text]
pTypes
| Prelude.length names == 1 = dropEnd1 $ map wrapHaskell prettyTypes
| otherwise = map wrapHaskell prettyTypes
pTypes :: M.Map Name Location -> [T.Text]
pTypes locationsMap =
case names of
[_singleName] -> dropEnd1 $ prettyTypes Nothing locationsMap
_ -> prettyTypes Nothing locationsMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit offtopic:

  • Why do we dropEnd1 when length names == 1?
  • Why don't we dropEnd1 when length names /= 1?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to remove this (actually I did but then reverted it) because it seems weird and unneeded to me as well. I tested a build without it and it seemed fine.

However, this is 4 year old code introduced by 2fef041 so I can't really comment.

Copy link
Contributor

@jian-lin jian-lin Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess for the motivation behind dropEnd1 is that the author wants to avoid showing the same type/signature in pTypes as the one shown in prettyName. If that is the case, I think we should also do dropEnd1 when length names /= 1 (for example, names has 1 "actual" name and an evidence name), at least in today's GHC versions.

I also need to deal with this duplication when implementing signature help. I decide to filter out the same type instead of dropEnd1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually dropEnd1 was introduced in 5dfee4c. Maybe @wz1000 has some ideas to share?

Copy link
Collaborator

@soulomoon soulomoon Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why just change it to use intelecate.

@fendor fendor self-requested a review August 20, 2025 07:55
@fendor fendor added the status: needs review This PR is ready for review label Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soulomoon soulomoon soulomoon left review comments

@wz1000 wz1000 Awaiting requested review from wz1000 wz1000 is a code owner

@fendor fendor Awaiting requested review from fendor

+1 more reviewer

@jian-lin jian-lin jian-lin left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status: needs review This PR is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dnikolovv @dschrempf @fendor @soulomoon @jian-lin